Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lens]Do not enable histogram mode for multiple un-stacked bar series #78525

Merged
merged 5 commits into from
Oct 2, 2020

Conversation

PavithraCP
Copy link
Contributor

@PavithraCP PavithraCP commented Sep 25, 2020

Summary

Fix for Issue #72243
Do not enable histogram mode for multiple bar series in case of un-stacked bar charts. Please refer the discussion below for more details.

The output of the bug in #72243 after applying this fix is as follows:

Screenshot from 2020-10-01 02-29-03

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@PavithraCP PavithraCP requested a review from a team September 25, 2020 03:33
@kibanamachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@PavithraCP PavithraCP changed the title Do not enable histogram mode for multiple layers Do not enable histogram mode for multiple layers& metrics Sep 25, 2020
@PavithraCP PavithraCP changed the title Do not enable histogram mode for multiple layers& metrics [Lens]Do not enable histogram mode for bar chart in case of multiple layers& metrics Sep 25, 2020
@timroes
Copy link
Contributor

timroes commented Sep 25, 2020

Jenkins, test this

@flash1293
Copy link
Contributor

flash1293 commented Sep 25, 2020

Seems like a few things are not right yet, let me know when you need help fixing them.

About the logic itself: This wasn’t explicitly stated in the issue, but the chart should still use histogram mode even if there are multiple series as long as it’s in stacked mode.

Histogram mode means the width of the bars in the chart corresponds to the interval the value takes up on the x axis. This does work fine for stacked bars (because stacking only changes height, not width), but it doesn’t when multiple bars are shown next to each other for the same x axis value because they would overlap. That’s why we have to disable histogram mode for explicitly unstacked bar charts if they have multiple series

@PavithraCP
Copy link
Contributor Author

That makes sense. My understanding from the issue description,

This logic is missing the multiple layer case and the multiple metric case - in both cases we can't use histogram mode.

was that we need to disable histogram mode in either of two cases: the chart has multiple layers or multiple metrics, irrespective of the seriesType/stack mode.

Based on your explanation, we need to disable histogram mode in case of unstacked charts with more than one series, i.e (!seriesType.includes['stacked'] && chartHasMoreThanOneSeries) . So the new logic would be like:
isHistogram && (seriesType.includes('stacked') || !splitAccessor) && ! (!seriesType.includes['stacked'] && chartHasMoreThanOneSeries). Please let me know if this sounds good

@flash1293
Copy link
Contributor

Sounds better - actually, now that I’m thinking about it a bit more, there is something else to consider. As long as the multiple series are area or line series, we still can use histogram mode as those don’t mess with the x axis width. So we can’t use chartHasMoreThanOneSeries directly but we have to check whether there are multiple unstacked bar series specifically.

@PavithraCP
Copy link
Contributor Author

@flash1293 I've pushed a commit with the logic to disable histogram mode for unstacked bar chart(s) with more than one series. The logic is a bit verbose now (for better readability), if needed, it can be simplified using boolean formulae. Hopefully, I've got the logic correct this time :) Please let me know.

One existing test - it applies histogram mode to the series for single series was modified too. The input used in the test was a multiple series (as per the logic for chartHasMoreThanOneSeries, since it had more than one accessor). This has been changed to a single accessor layer.

@flash1293
Copy link
Contributor

flash1293 commented Sep 29, 2020

@PavithraCP Thanks for updating! I think there is one more case missing though - your logic is using chartHasMoreThanOneSeries and disables histogram mode if the current series is an unstacked bar and there is more than one series.

The problem is the following case: you have an unstacked bar series (without split) together with a line series or area series in a second layer. In your PR this would disable histogram mode (because there is more than one series in the chart), but actually it would work fine because the line/area series can be displayed on top of the single bar series without interfering with the x axis width. So instead of relying on the chartHasMoreThanOneSeries variable you need to check whether there is more than one bar series by iterating through all of the layers and checking whether there is more than one bar series. The rest of the logic looks right, it’s basically replacing your usage of chartHasMoreThanOneSeries by “chartHasMoreThanOneBarSeries”

Also, could you update your PR by merging in current master from the elastic/kibana branch?

Thanks again for sticking with it 💚

@PavithraCP
Copy link
Contributor Author

@flash1293 Implemented the suggested change and re-based the branch to latest master. Also ran the new logic against the bug faced in the issue and checked if the behavior is as expected. I've attached the screenshot in the PR summary. Please let me know if the logic looks good. Thanks!

@PavithraCP PavithraCP changed the title [Lens]Do not enable histogram mode for bar chart in case of multiple layers& metrics [Lens]Do not enable histogram mode for multiple un-stacked bar series Oct 1, 2020
@flash1293
Copy link
Contributor

@elasticmachine merge upstream

@flash1293
Copy link
Contributor

Jenkins, test this

@flash1293 flash1293 self-assigned this Oct 2, 2020
@flash1293 flash1293 added Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0 labels Oct 2, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in Chrome and works as expected. LGTM if the build comes out green.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
lens 1010.4KB 1010.8KB +405.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@flash1293
Copy link
Contributor

I'm going to merge, thanks for your contribution 💚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants